Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Javascript in status.html to separate file #131

Closed
wants to merge 10 commits into from
Closed

Move Javascript in status.html to separate file #131

wants to merge 10 commits into from

Conversation

jakethakur
Copy link

Moves the javascript in status.html to a new status.js file.

Closes #123

@manu-chroma
Copy link
Owner

Hey Jake. You can click on detail to check the build log for travis for your particular PR. Check it out here: https://travis-ci.org/manu-chroma/username-availability-checker/builds/449037018?utm_source=github_status&utm_medium=notification for the current failing build.

Also, try testing your changes locally. That might help you in knowing what might have gone wrong (apart from the logs)

@jakethakur
Copy link
Author

OK, I believe it was just that I left some trailing whitespace (though the log is fairly verbose so I might have missed something else further down).

@manu-chroma
Copy link
Owner

I kind of agree. The log went just too verbose on failure. Some of the initial stuff was not very useful as such. Maybe we can do something about that later.

Copy link
Owner

@manu-chroma manu-chroma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jakethakur
Copy link
Author

Great, thank you for your help!

@manu-chroma
Copy link
Owner

I will let someone else have a look too, before I merge this one!

Copy link
Contributor

@kx-chen kx-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small issue: there are template tags (which would normally be rendered by flask) in the Javscript which do not get rendered when they are moved to a separate file. This causes the entire site to break. Not @jakethakur 's fault. Looking into this more. Other than that, it looks good to me!

Copy link
Contributor

@kx-chen kx-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

static/js/status.js Outdated Show resolved Hide resolved
static/js/status.js Outdated Show resolved Hide resolved
@jakethakur
Copy link
Author

OK, that should have fixed it :)

Copy link

@utkarsh2102 utkarsh2102 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :D

@manu-chroma
Copy link
Owner

We have to consider something like this: https://stackoverflow.com/questions/37259740/passing-variables-from-flask-to-javascript/37260465

or maybe a better solution for js file to have that config.

@kx-chen
Copy link
Contributor

kx-chen commented Nov 1, 2018

I've got a solution that's similiar to @manu-chroma 's suggestion. Can submit PR if you'd like. My thoughts are that it should be addressed in a different issue/pr, as it is a bit out of the scope of this task.

@ghost
Copy link

ghost commented Dec 15, 2018

how to answer this question? im new

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Javascript in html files to separate file
5 participants